Support common google apis and fix issue with ";" in package names #26
Conversation
|
so what does it mean with semicolon? does it propose us to use it as an alias? |
|
Yeah, it's basically an alias that allows you to specify a directory path that's different from the package name. See: golang/protobuf#139 Not supporting it creates an issue when using standard protobuf dependencies like: https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/descriptor.proto |
protoc-gen-gripmock/generator.go
Outdated
| @@ -169,6 +169,10 @@ func resolveDependencies(protos []*descriptor.FileDescriptorProto) map[string]st | |||
| continue | |||
| } | |||
|
|
|||
| // some package declarations use a semicolon, which causes a formatting error | |||
| // ignore for now | |||
| pkg = strings.Split(pkg, ";")[0] | |||
There was a problem hiding this comment.
so here we should check wether it's contained a dedicated alias or not, if yes then use it as alias.
|
also please provide on how this has been tested. if possible update the unit test to cover this case. |
|
@jekiapp updated it to use alias instead I tested by adding And verified that the server behaves as expected. Without the change the server fails to start with the error specified above. Unfortunately I'm pretty new to Go and I'm having trouble getting the tests to compile and run correctly, so haven't had luck with the unit test. If you can send me instructions to get my local build setup on a mac, I can give it another shot. |
|
will test your PR today in my local |
| @@ -7,6 +7,7 @@ RUN mkdir /stubs | |||
| RUN apk -U --no-cache add git protobuf | |||
|
|
|||
| RUN go get -u -v github.com/golang/protobuf/protoc-gen-go \ | |||
| github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway \ | |||
There was a problem hiding this comment.
First I need to know whether this is a common package or not, otherwise I'll find a way how to add this kind of custom dependecies
There was a problem hiding this comment.
Another option would be to get the same protos from this repo instead: https://github.com/googleapis/api-common-protos
These are pretty common protos advocated by Google as part of their API design guide, so it's used by a lot of folks.
A more generic solution would be to have an option to pass in a specific protodir (that's mounted on the image), instead of just defaulting the protodir to the parent directory of the mocked proto file, this lets users be able to pull in all the dependencies in their entire proto project if they want.
i.e. my folder structure is:
protos/
├── google/
│ ├── http.proto
├── identity/
│ ├── auth.proto
If I mocked auth.proto, gripmock wouldn't be able to find http.proto as it sets protodir to protos/identity/, excluding the google folder. But If I could manually set protodir to protos/, that will fix the issue and give a more flexible way for users to specify dependencies.
There was a problem hiding this comment.
I just created pr #28 to cover this case. please check it out. with that, user should be able to derive their own dockerfile and install their own dependencies then pass the custom protos with --imports arg
|
Meanwhile I just merged the fix of the original issue of this PR, sory I didn't use your PR since I also add integration test on #27 to prove my solution (support ; in go package) |
|
@jekiapp thanks, looks good! |
I noticed that if you try to start a service with a proto file that includes a dependency on something with package like
go_package=github.com/golang/protobuf/protoc-gen-go/descriptor;descriptor, it fails with:This is because
server.tmpljust naively uses the package declaration as the import statement, which results in invalid like above. I fixed truncating the package declaration at the semicolon.I also propose to automatically include common google apis as part of Proto Path as I find them very commonly used in services, so it would be a nice convenience feature.